Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Token request attribute configurable #89

Merged
merged 3 commits into from
Mar 10, 2017
Merged

Token request attribute configurable #89

merged 3 commits into from
Mar 10, 2017
+48 −11

Conversation

basz
Copy link
Collaborator

@basz basz commented Mar 9, 2017

No description provided.

@basz basz requested a review from prolic March 9, 2017 14:49
@coveralls
Copy link

coveralls commented Mar 9, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 3abf61d on basz:token-request-attribute-configurable into d510e76 on zf-fr:master.

@prolic
Copy link
Collaborator

prolic commented Mar 10, 2017

On PHP 7.1: There was 1 failure:

  1. ZfrOAuth2Test\Server\Grant\PasswordGrantTest::testCanCreateTokenResponse with data set Security audit/code review #1 (false)
    Failed asserting that 3599 matches expected 3600.

prolic
prolic reviewed Mar 10, 2017
View reviewed changes
src/Middleware/ResourceServerMiddleware.php Outdated
* @param ResourceServerInterface $resourceServer
*/
public function __construct(ResourceServerInterface $resourceServer)
public function __construct(ResourceServerInterface $resourceServer, ServerOptions $serverOptions)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe allow null for server options and use default then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I saw too this but didn't want to change it all over the place again.

But if I do I think it is be better to simply pass the option itself, unless there are more then 3 or 4 constructor arguments or the instance needs to pass them on to other dependencies. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@basz basz force-pushed the token-request-attribute-configurable branch from 3abf61d to bbbe022 Compare March 10, 2017 10:48
@coveralls
Copy link

coveralls commented Mar 10, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.005%) to 99.588% when pulling bbbe022 on basz:token-request-attribute-configurable into bad8846 on zf-fr:master.

@prolic prolic merged commit b559938 into zf-fr:master Mar 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prolic prolic prolic left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@basz @coveralls @prolic